Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transparent compiler #1240

Merged
merged 60 commits into from
Apr 23, 2024
Merged

Conversation

TheAngryByrd
Copy link
Member

@TheAngryByrd TheAngryByrd commented Mar 1, 2024

Adds support for Transparent Compiler in FSAC.

@TheAngryByrd TheAngryByrd force-pushed the transparent-compiler branch from 4f6ccba to 0242c14 Compare March 15, 2024 19:17
src/FsAutoComplete.Core/AdaptiveExtensions.fs Fixed Show resolved Hide resolved
src/FsAutoComplete.Core/AdaptiveExtensions.fs Fixed Show fixed Hide fixed
@@ -680,6 +730,7 @@
let bind (mapping: 'a -> CancellationToken -> asyncaval<'b>) (value: asyncaval<'a>) =
let mutable cache: option<_> = None
let mutable innerCache: option<_> = None
let mutable outerDataCache: option<_> = None

Check notice

Code scanning / Ionide.Analyzers.Cli

Detect if generic type should be in the postfix position. Note

Prefer postfix syntax for options.
src/FsAutoComplete.Core/AdaptiveExtensions.fs Fixed Show fixed Hide fixed
@TheAngryByrd TheAngryByrd force-pushed the transparent-compiler branch 2 times, most recently from 42c2357 to bcaabe9 Compare March 15, 2024 22:01
@TheAngryByrd TheAngryByrd force-pushed the transparent-compiler branch from e0dbfbf to 8226064 Compare March 26, 2024 03:05
@@ -18,10 +18,13 @@
module LanguageVersionShim =
val defaultLanguageVersion: Lazy<LanguageVersionShim>


val fromOtherOptions: options: seq<string> -> LanguageVersionShim

Check notice

Code scanning / Ionide.Analyzers.Cli

Detect if generic type should be in the postfix position. Note

Prefer postfix syntax for sequences.

member ScriptTypecheckRequirementsChanged: IEvent<unit>

member RemoveFileFromCache: file: string<LocalPath> -> unit

member ClearCache: snap: seq<FSharpProjectSnapshot> -> unit

Check notice

Code scanning / Ionide.Analyzers.Cli

Detect if generic type should be in the postfix position. Note

Prefer postfix syntax for sequences.
@TheAngryByrd TheAngryByrd force-pushed the transparent-compiler branch 4 times, most recently from 381198b to acbfe87 Compare April 17, 2024 03:20
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
Comment on lines +40 to +49
real.ContinueWith(fun (task: Task<_>) ->
match task.Status with
| TaskStatus.RanToCompletion -> tcs.TrySetResult task.Result |> ignore<bool>
| TaskStatus.Canceled ->
tcs.TrySetCanceled(TaskCanceledException(task).CancellationToken)
|> ignore<bool>
| TaskStatus.Faulted -> tcs.TrySetException(task.Exception.InnerExceptions) |> ignore<bool>

| _ -> ())
|> ignore<Task>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed back to ContinueWith style as getting stack traces weren't working properly. Wish I had a good idea how to make a test for that.

src/FsAutoComplete.Core/AdaptiveExtensions.fs Show resolved Hide resolved
src/FsAutoComplete.Core/AdaptiveExtensions.fs Show resolved Hide resolved
Comment on lines +237 to +251
match! inMemorySourceFiles |> AMap.tryFind sourcePath with
| Some volatileFile -> return! volatileFile |> AVal.map createFSharpFileSnapshotInMemory
| None -> return! createFSharpFileSnapshotOnDisk sourceTextFactory sourcePath
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to source files, either in memory or on disk will cause snapshots to be recreated.

>> Log.addContextDestructured "projectFileName" p.ProjectFileName
)

snapshot
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finding an already created snapshot keeps memory use down.

Comment on lines +56 to +68
let! projectFileName = projectFileName
and! projectId = projectId
and! sourceFiles = sourceFiles
and! referencePaths = referencePaths
and! otherOptions = otherOptions
and! referencedProjects = referencedProjects
and! isIncompleteTypeCheckEnvironment = isIncompleteTypeCheckEnvironment
and! useScriptResolutionRules = useScriptResolutionRules
and! loadTime = loadTime
and! unresolvedReferences = unresolvedReferences
and! originalLoadReferences = originalLoadReferences
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, any changes to any of these could recreate a snapshot. In practice only a few of these change

referencePath.Substring(3) // remove "-r:"
|> createReferenceOnDisk)

let referencedProjects = mapReferences p
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where it creates/finds referenced snapshots, and when those change it will recreate the current snapshot.


member DisableInMemoryProjectReferences: bool with get, set

static member GetDependingProjects:
file: string<LocalPath> ->
options: seq<string * FSharpProjectOptions> ->
(FSharpProjectOptions * FSharpProjectOptions list) option
snapshots: seq<string * CompilerProjectOption> ->

Check notice

Code scanning / Ionide.Analyzers.Cli

Detect if generic type should be in the postfix position. Note

Prefer postfix syntax for sequences.
options: seq<string * FSharpProjectOptions> ->
(FSharpProjectOptions * FSharpProjectOptions list) option
snapshots: seq<string * CompilerProjectOption> ->
option<CompilerProjectOption * list<CompilerProjectOption>>

Check notice

Code scanning / Ionide.Analyzers.Cli

Detect if generic type should be in the postfix position. Note

Prefer postfix syntax for options.
options: seq<string * FSharpProjectOptions> ->
(FSharpProjectOptions * FSharpProjectOptions list) option
snapshots: seq<string * CompilerProjectOption> ->
option<CompilerProjectOption * list<CompilerProjectOption>>

Check notice

Code scanning / Ionide.Analyzers.Cli

Detect if generic type should be in the postfix position. Note

Prefer postfix syntax for lists.

member GetProjectOptionsFromScript:
file: string<LocalPath> * source: ISourceText * tfm: FSIRefs.TFM ->
Async<FSharpProjectOptions * FSharpDiagnostic list>
Async<FSharpProjectOptions * list<FSharp.Compiler.Diagnostics.FSharpDiagnostic>>

Check notice

Code scanning / Ionide.Analyzers.Cli

Detect if generic type should be in the postfix position. Note

Prefer postfix syntax for lists.
file: string<LocalPath> * snapshot: FSharpProjectSnapshot -> ParseAndCheckResults option

member TryGetRecentCheckResultsForFile:
file: string<LocalPath> * options: FSharpProjectOptions * source: ISourceText -> option<ParseAndCheckResults>

Check notice

Code scanning / Ionide.Analyzers.Cli

Detect if generic type should be in the postfix position. Note

Prefer postfix syntax for options.
Async<FSharpSymbolUse array>

member FindReferencesForSymbolInFile:
file: string * project: FSharpProjectOptions * symbol: FSharpSymbol -> Async<seq<range>>
file: string<LocalPath> * project: FSharpProjectSnapshot * symbol: FSharpSymbol -> Async<seq<range>>

Check notice

Code scanning / Ionide.Analyzers.Cli

Detect if generic type should be in the postfix position. Note

Prefer postfix syntax for sequences.
file: string<LocalPath> * project: FSharpProjectSnapshot * symbol: FSharpSymbol -> Async<seq<range>>

member FindReferencesForSymbolInFile:
file: string<LocalPath> * project: FSharpProjectOptions * symbol: FSharpSymbol -> Async<seq<range>>

Check notice

Code scanning / Ionide.Analyzers.Cli

Detect if generic type should be in the postfix position. Note

Prefer postfix syntax for sequences.

member __.DisableInMemoryProjectReferences
with get () = disableInMemoryProjectReferences
and set (value) = disableInMemoryProjectReferences <- value

static member GetDependingProjects (file: string<LocalPath>) (options: seq<string * FSharpProjectOptions>) =
static member GetDependingProjects (file: string<LocalPath>) (snapshots: seq<string * CompilerProjectOption>) =

Check notice

Code scanning / Ionide.Analyzers.Cli

Detect if generic type should be in the postfix position. Note

Prefer postfix syntax for sequences.
@@ -9,16 +9,15 @@
[<RequireQualifiedAccess; NoComparison>]
type SymbolDeclarationLocation =
| CurrentDocument
| Projects of FSharpProjectOptions list * isLocalForProject: bool
| Projects of CompilerProjectOption list * isLocalForProject: bool

Check notice

Code scanning / Ionide.Analyzers.Cli

Verifies each field in a union case is named. Note

Field inside union case is not named!
@TheAngryByrd TheAngryByrd force-pushed the transparent-compiler branch from 43076cc to ae52948 Compare April 21, 2024 22:16
@TheAngryByrd
Copy link
Member Author

Ok I think this is finally ready. This can now be turned on/off so we can merge safely without fear of being able to use the old stuff since transparent compiler may still have issues that we have no control of directly.

@TheAngryByrd TheAngryByrd force-pushed the transparent-compiler branch from 8e6b98e to 8696c11 Compare April 22, 2024 03:26
@nojaf
Copy link
Contributor

nojaf commented Apr 22, 2024

How do we play with this? I checked out your branch, built and tried the following:

image

Or should --use-fcs-transparent-compiler be added somewhere else?

@nojaf
Copy link
Contributor

nojaf commented Apr 22, 2024

Ok, $env:USE_TRANSPARENT_COMPILER = 'TransparentCompiler' did the trick.
I tried this on three projects, it looks very promising!
You can tell by how fast semantic highlighting is kicking in.

I'd be a fan of merging this into the nightly branch and playing with it for a while.

@TheAngryByrd
Copy link
Member Author

How do we play with this? I checked out your branch, built and tried the following:

Or should --use-fcs-transparent-compiler be added somewhere else?

Unfortunately there isn't a way to add generic parameters via Ionide. (means that should get added at some point). I do have a PR available in Ionide to add it but you'll have to build a run a custom version of Ionide to access the setting.

Ok, $env:USE_TRANSPARENT_COMPILER = 'TransparentCompiler' did the trick. I tried this on three projects, it looks very promising! You can tell by how fast semantic highlighting is kicking in.

Are you sure that actually works? I'm only using USE_TRANSPARENT_COMPILER it in the tests to divide up test runs.

@vzarytovskii
Copy link

vzarytovskii commented Apr 22, 2024

USE_TRANSPARENT_COMPILER has no effect on anything in the compiler/service itself, we for sure don't use this particular variable anywhere. I think this PR uses it in tests only.

Copy link
Contributor

@baronfel baronfel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few notes and a couple questions as I was reading through this.

I'm surprised by how understandable this was, despite the large amount of changes. I think this says a lot about the comments you left and the places you unified previous logic.

Directory.Build.props Outdated Show resolved Hide resolved
src/FsAutoComplete.Core/AdaptiveExtensions.fs Show resolved Hide resolved
let! ct = CancellableTask.getCancellationToken ()
let inner = mapping i ct
return inner
RefCountingTaskCreator(fun ct ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the same throughout this file? moving from cancellableTask to task for better stack traces?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I need to rundown why I was still getting weird stacktraces but I'll do that later.

src/FsAutoComplete.Core/Commands.fs Show resolved Hide resolved
src/FsAutoComplete.Core/FileSystem.fs Show resolved Hide resolved
Comment on lines +6 to +9
/// <summary>
/// An awaitable wrapper around a task whose result is disposable. The wrapper is not disposable, so this prevents usage errors like "use _lock = myAsync()" when the appropriate usage should be "use! _lock = myAsync())".
/// </summary>
[<Struct>]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beautiful type - thanks for this

src/FsAutoComplete.Core/SymbolLocation.fs Outdated Show resolved Hide resolved
src/FsAutoComplete/CodeFixes.fsi Outdated Show resolved Hide resolved
@@ -418,7 +433,7 @@ type AdaptiveState(lspClient: FSharpLspClient, sourceTextFactory: ISourceTextFac


let runAnalyzers (config: FSharpConfig) (parseAndCheck: ParseAndCheckResults) (volatileFile: VolatileFile) =
async {
asyncEx {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the direct use of async be a code smell for us now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, though we can make additional PRs to hit all those places.



let compilers =
match getEnvVarAsStr "USE_TRANSPARENT_COMPILER" with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheAngryByrd, hmm, ok now I'm doubting whether that did work or not.
It might not have and I was seeing things

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I hardcoded the useTransparentCompiler to true locally and I remain with my past statements: all goes well for me, ship it.

@TheAngryByrd TheAngryByrd merged commit c5a5812 into ionide:nightly Apr 23, 2024
23 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants